-
Notifications
You must be signed in to change notification settings - Fork 230
GIT-521: add support for sanic framework #1056
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GIT-521: add support for sanic framework #1056
Conversation
💔 Tests Failed
Expand to view the summary
Build stats
Test stats 🧪
Test errors
Expand to view the tests failures> Show only the first 10 test failures
|
a45917f
to
4a9c541
Compare
Hi @beniwohli @basepi I would really appreciate it if you guys would take a look at this and see if things are going in the right direction for this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harshanarayana Great Work! Apart from a few small things, this looks great!
elasticapm/contrib/sanic/__init__.py
Outdated
:param request: Sanic HTTP Request object | ||
:return: string containing the Transaction name | ||
""" | ||
return f"{request.method} {request.path}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally try to get a parametrized version of the path (a.k.a "route") for the transaction name (so that /books/123
and /books/42
are tracked under the same transaction /books/<id>
or similar).
Having a quick glance at the source code, request.app.router.get(request)[0].raw_path
might be what we're looking for. Unfortunately, that triggers the resolver a second time, but there's an LRU cache on it, so the incurred overhead should be minimal. We might however want to catch the exceptions that Router._get
raises.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me quickly test the difference in overhead caused by this change of moving from current to anything else that can get the url as /books/<id>
. However, if you notice, there is a way to customize this behavior that is provided to the end user already using transaction_name_callback
argument while creating the ElasticAPM
instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me use request.uri_template
instead. This give me '/stuff/<info:int>'
format for the URL. Do you think I should strip the :int
or that is good to have ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using request.uri_template in my custom instrumentation and it looks good. I would personally like to have param type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reopening this item back again. The uri_template
has a small behavior that makes it not really suitable. I did a quick test to confirm my suspicion. It breaks the span
generation workflow. Will find a better suitable alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
url_template = request.path
# Sanic's new router puts this into the request itself so that it can be accessed easily
if hasattr(request, "route"):
url_template = request.route.path
else:
# Let us fallback to using old router model to extract the info
try:
_, _, _, url_template, _, _ = self._app.router.get(request=request)
except:
pass
return f"{request.method} {url_template}"
Falling back to this way to make sure we are compatible with both version of sanic
as the revamped router has changed a bit in how the data is accessed.
@ahopkins Does this look all right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have the request
object, why invoke the router again in the pre-21.3 block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One quick thing worth mentioning, the request.route.path
string will not have the first /
in the path. Not sure if this matters for your usage.
3d516c7
to
6291aa9
Compare
@harshanarayana I ran the tests on our test suite. You can ignore the failures on Python 3.10 (there seems to be a compilation issue with httptools on that unreleased version of Python, see MagicStack/httptools#54), but the others look like legitimate failures: |
Thanks a lot for the check @beniwohli Let me check and see what is broken and fix and update the PR. |
Sorry, @beniwohli I have been away again on some personal matters. I will rebase the changes again do a quick check with latest version of |
@harshanarayana what's the update here? is there anything pending on this one? |
@beniwohli what is the plan for this PR? |
Hey @ajay1mg, sorry for the lack of response here. We're currently re-evaluating how we prioritize this kind of work. Community contributions, especially of large features and integrations, are a bit fraught -- while they can seem like a slam-dunk, by accepting the contribution we are agreeing to support it into the future. So we need to make time to really check that it's solid and that we understand the underlying technologies before merging. Plus documentation! It's a lot of work that we need to actually roadmap and prioritize. Thanks for your patience! |
Any update about this merge? A lot of people is currently waiting this integration with SANIC to be more cleanest in the code and be in the same boat of elasticsearch APM implementations. Thx |
@soulcodex We have this on our roadmap. We need to spend the time to research Sanic for ourselves and make sure everything is good with this integration, since by merging this we'll be committing to maintaining it. Again, apologies for the long delay. |
I agree with @soulcodex, I personally use Sanic for my personal projects and know a lot of people using it. Also, if you look at the above comments, it was near ready to be merged. |
@basepi I am a maintainer of Sanic, and the original author of the PR is also a core developer of Sanic. Please LMK if you need any support from me or have any questions re: the project. |
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My apologies for our long delays in reviewing this PR. It's an excellent addition! Couple of nits below and we'll get this tested and merged. 🎉
"SECRET_TOKEN": "supersecrettokenstuff", | ||
}) | ||
Pass a pre-build Clinet instance to the APM Middleware:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass a pre-build Clinet instance to the APM Middleware:: | |
Pass a pre-build Client instance to the APM Middleware:: |
pytest-mock ; python_version >= '3.7' | ||
httpx ; python_version >= '3.6' | ||
|
||
sanic ; python_version >= '3.6' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be >= '3.7'
instead? In the jenkins exclude you have excluded python 3.6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is correct. Sanic does not support 3.6
include::./tornado.asciidoc[] | ||
|
||
include::./starlette.asciidoc[] | ||
include::./starlette.asciidoc[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include::./starlette.asciidoc[] | |
include::./starlette.asciidoc[] | |
include::./sanic.asciidoc[] |
Looks like a bunch of tests are failing (now that we're actually running them). I haven't looked at the failures yet -- please ping me if you have questions about the failures. |
@harshanarayana Are you going to be able to look at these test failures? I'd love to get this fixed up and merged. |
@ahopkins did anything changed/deprecated in sanic 21.9? but breaks for 21.9 with following error:
looks like something in registering a custom error handler? |
Usually, the fix is to just add: I added something here recently as well: getsentry/sentry-python#1212 I can take a look at the tests to see if I can help with the same caveat I offered for Sentry: I do not know the product so I may need some help/guidance, and since I do not have write access to @harshanarayana repo, I guess I can either provide a diff, or make another PR? |
|
Assuming it was that simple: These were the only changes that I made: diff --git a/docs/set-up.asciidoc b/docs/set-up.asciidoc
index 4df452e8..15269e0f 100644
--- a/docs/set-up.asciidoc
+++ b/docs/set-up.asciidoc
@@ -23,4 +23,6 @@ include::./tornado.asciidoc[]
include::./starlette.asciidoc[]
+include::./sanic.asciidoc[]
+
include::./serverless.asciidoc[]
diff --git a/elasticapm/contrib/sanic/patch.py b/elasticapm/contrib/sanic/patch.py
index 78c1bb2f..72986d59 100644
--- a/elasticapm/contrib/sanic/patch.py
+++ b/elasticapm/contrib/sanic/patch.py
@@ -50,11 +50,11 @@ class ElasticAPMPatchedErrorHandler(ErrorHandler):
self._current_handler = current_handler # type: ErrorHandler
self._apm_handler = None # type: ApmHandlerType
- def add(self, exception, handler):
- self._current_handler.add(exception, handler)
+ def add(self, exception, handler, *args, **kwargs):
+ self._current_handler.add(exception, handler, *args, **kwargs)
- def lookup(self, exception):
- return self._current_handler.lookup(exception)
+ def lookup(self, exception, *args, **kwargs):
+ return self._current_handler.lookup(exception, *args, **kwargs)
def setup_apm_handler(self, apm_handler: ApmHandlerType, force: bool = False):
if self._apm_handler is None or force: diff --git a/elasticapm/contrib/sanic/__init__.py b/elasticapm/contrib/sanic/__init__.py
index 0d8b631b..991ce69d 100644
--- a/elasticapm/contrib/sanic/__init__.py
+++ b/elasticapm/contrib/sanic/__init__.py
@@ -86,7 +86,7 @@ class ElasticAPM:
"SECRET_TOKEN": "supersecrettokenstuff",
})
- Pass a pre-build Clinet instance to the APM Middleware::
+ Pass a pre-build Client instance to the APM Middleware::
>>> apm = ElasticAPM(app=app, client=Client())
diff --git a/setup.cfg b/setup.cfg
index 1f2c48e1..e6a9d938 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -96,7 +96,7 @@ tests_require =
pytest-asyncio ; python_version >= '3.7'
pytest-mock ; python_version >= '3.7'
httpx ; python_version >= '3.6'
- sanic ; python_version >= '3.6'
+ sanic ; python_version >= '3.7'
[options.extras_require]
flask = |
that's all it took to pass the tests |
Closing in favor of #1390 which will merge shortly 🚀 |
What does this pull request do?
Replacement for #522
Related issues
closes #521
Todo